Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Prevent infinite buffering at the start of looped video on edge #392

Merged
merged 1 commit into from
Mar 1, 2019

Conversation

brandonocasey
Copy link
Contributor

Problem

Right now Edge will sometimes buffer indefinitely once a video that is set to loop moves back to currentTime 0.

After investigating this it appears that Edge does not set tech.seeking() to true during a seeking event for looping back to currentTime 0. Normal seeking events do set tech.seeking() to true, as expected.

Solution

I don't think that we need the extra check for seeking on the tech at the start of the if statement. If we just had a seeking event it should be fairly evident that we are seeking. I also think that the other two checks (for currentTime, and loop) should be enough to have this only happen when a video is looping.

@forbesjo
Copy link
Contributor

I wonder if the problem is by the time the callback is called the video element has resolved the seek and seeking is no longer true. This looks safe to me.

@forbesjo
Copy link
Contributor

Perhaps we should consider either reverting #161 and only react to seek events or very clearly document the instances where the event is unreliable.

@brandonocasey
Copy link
Contributor Author

I think the middleware approach makes more sense. Maybe it just needs to be documented somewhere that a seek with looping video will not go through middleware?

@brandonocasey brandonocasey mentioned this pull request Feb 21, 2019
@misteroneill misteroneill merged commit b6d1b97 into master Mar 1, 2019
@misteroneill misteroneill deleted the fix/loop-buffering-edge branch March 1, 2019 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants